Conversation
commit: |
dreyfus92
left a comment
There was a problem hiding this comment.
awesome job @AlexanderKaran 😄 left a few comments
|
cc. @43081j 👀 |
Co-authored-by: paul valladares <85648028+dreyfus92@users.noreply.github.com>
| 'lib/**' | ||
| ]; | ||
|
|
||
| const IMPORT_RE = |
There was a problem hiding this comment.
we might be better off using es-module-lexer to actually parse export/import and do it that way.
but not sure where we'd be with require then 🤔 so maybe not...
|
super nice! just a couple of comments mostly we should decide if |
Great @dreyfus92 what are your thoughts? |
dreyfus92
left a comment
There was a problem hiding this comment.
hey @AlexanderKaran awesome job, left you a few comments 😄
| ]; | ||
|
|
||
| const IMPORT_RE = | ||
| /(?:import\s+(?:.*\s+from\s+)?|require\s*\()\s*['"]([^'"]+)['"]/g; |
There was a problem hiding this comment.
we should add some tests for this which is likely to produce false positives. \s+from\s+ can span across multiple imports on the same file, misassociating the import keyword with a later from "..." i agree with james suggestion about using the es-module-lexer to actually parse all of this.
There was a problem hiding this comment.
So I am pretty sure @43081j is right, and we would lose support for picking up require('core-js/...'). Do you think we need to keep that?
Done |
|
@dreyfus92 @43081j Pushed a few updates and replied about the |
Moved my hacky version of detecting CoreJS into the CLI. Will only check build folder if you pass in a flag for now. Example output from testing a package from the framework tracker: